Re: Gphoto-devel Digest, Vol 95, Issue 12

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Gphoto-devel Digest, Vol 95, Issue 12

Theodore Kilgore
> Date: Fri, 11 Apr 2014 23:50:01 +0200
> From: Marcus Meissner <[hidden email]>
> Subject: Re: [gphoto-devel] [PATCH 00/17] Fix all compiler warnings on
> Linux x86_64 native
> To: [hidden email]
> Cc: [hidden email]
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset=us-ascii
>
> On Fri, Apr 11, 2014 at 07:28:25PM +0100, [hidden email] wrote:
> > From: "Daniel P. Berrange" <[hidden email]>
> >
> > This series attempts to fix or silence all compiler warnings
> > that show up when doing a native Linux build on a Fedora 20
> > x86_64 host, gcc 4.8.2. It is entirely possible that other
> > Linux versions/distros will still show bugs, due to different
> > gcc versions.
> >
> > The vast majority are a result of 'char *' vs 'unsigned char *'
> > inconsistencies in the APIs, so just require adding casts. As
> > I silenced these false positives, increasing signal/noise ratio,
> > the small number of genuine bugs became visible which I also
> > fixed here.
> >
> > With native building cleanly, I can also now see all the real
> > Win32 cross-compile bugs, which I'll focus on fixing next....
>
> I have applied all of it (excepting the ones I commented on, where
> I fixed it a bit differently ;),
>
> Builds without warnings here.
>
> Thanks :)
>
> Ciao, Marcus

Dan, Marcus,

I am very glad that someone has taken it upon himself to do this. It will
be nice to compile without generating all those warnings. My previous
impression, though, was that cruft had accumulated over time and that
nobody dared to touch this stuff, for fear that it might break something,
and somebody a long time ago wrote the code and is no longer here, etc.

I would like to make sure myself that everything is OK, but I won't have
time to concentrate on this until a couple of weeks from now, when the
academic term ends. In order to do the testing then, though, I would like
to ask that to what code base they are to be applied. In particular, I am
pretty sure I remember that there was to be a move away from SVN (and
possibly a move away from Sourceforge?) and the setting up of a git tree.
What is the status of this?

In particular I would also like to mention some of the problems that I
noticed in the now not so recent past, because some of my camera drivers
are patched, I notice:

1. Often, inside of a camera driver it seems more stable to use unsigned
char than to use char in the various USB commands. I do not exactly know
why this happened, but I did notice. However, all the basic USB functions
are using char, not unsigned char. So in all or almost all of my camera
drivers the unsigned char variables have received casts.

2. As a case in point about ancient stuff that nobody can do much to
handle these days, consider the iclick camlib. I have only known of one
camera which is supported. It was onwned by someone in Spokane,
Washington (I live on the other side of the US, in Alabama), and i never
met him. The camlib was written based upon USB logs which were sent to me.
The owner of the camera (listed as co-author) disappeared from contact
many years ago. I have no idea how to contact him. Thus, there is no
feasible way to test revisions. I suspect that this is an extreme case,
but there are obviously similar ones here and there through the rest of
the camera libraries. How to make sure that everything is OK when there is
not camera to test and no known way to reach someone who owns one?

3. Most of the other cameras that I have supported I will be able to test
when I can get the free time to do the code patching.

Finally, I do repeat that I am glad this cleanup is happening.

Theodore Kilgore

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/NeoTech
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Gphoto-devel Digest, Vol 95, Issue 12

Daniel P. Berrange
On Tue, Apr 15, 2014 at 05:12:06PM -0500, Theodore Kilgore wrote:
> I am very glad that someone has taken it upon himself to do this. It will
> be nice to compile without generating all those warnings. My previous
> impression, though, was that cruft had accumulated over time and that
> nobody dared to touch this stuff, for fear that it might break something,
> and somebody a long time ago wrote the code and is no longer here, etc.

There's certainly scope to break stuff in a cleanup, but I think there's
greater risk in not spotting real compile problems due to the large number
of warnings hiding actual problems. Thus far the cleanups have been pretty
straightforward though, so the risk is low.

> I would like to make sure myself that everything is OK, but I won't have
> time to concentrate on this until a couple of weeks from now, when the
> academic term ends. In order to do the testing then, though, I would like
> to ask that to what code base they are to be applied. In particular, I am
> pretty sure I remember that there was to be a move away from SVN (and
> possibly a move away from Sourceforge?) and the setting up of a git tree.
> What is the status of this?

Currently gphoto is still using SVN. That said I do my work using a hacked
GIT conversion of the SVN repo since GIT is easier to work with for me.

> 1. Often, inside of a camera driver it seems more stable to use unsigned
> char than to use char in the various USB commands. I do not exactly know
> why this happened, but I did notice. However, all the basic USB functions
> are using char, not unsigned char. So in all or almost all of my camera
> drivers the unsigned char variables have received casts.

I think it would make sense that 'unsigned char *' be used everywhere,
along with 'size_t' for any lengths. Internally most of the camera drivers
use 'unsigned char*' for their own APIs. The problems, primarily arise
when then interacting with the gp_port and the gp_file APIs which are
using 'char *' when they really ought to have been 'unsigned char *'.
The problem is the latter are in public header files, so changing
them to 'nusigned char *' would be an API change that could well cause
pain for applications using libgphoto. So I think we're stuck using
casts.

> 2. As a case in point about ancient stuff that nobody can do much to
> handle these days, consider the iclick camlib. I have only known of one
> camera which is supported. It was onwned by someone in Spokane,
> Washington (I live on the other side of the US, in Alabama), and i never
> met him. The camlib was written based upon USB logs which were sent to me.
> The owner of the camera (listed as co-author) disappeared from contact
> many years ago. I have no idea how to contact him. Thus, there is no
> feasible way to test revisions. I suspect that this is an extreme case,
> but there are obviously similar ones here and there through the rest of
> the camera libraries. How to make sure that everything is OK when there is
> not camera to test and no known way to reach someone who owns one?

Yeah, testing of cameras is a real hard problem. Even if you physical
cameras, testing cameras manually is a real time sink. I think it might
be possible to build a mock camera framework for testing. You'd have to
do something like create a test script, run it against a real camera and
record the USB traffic in some manner. Then the test suite would fake
the USB interaction replaying the recorded USB traffic during the test.
I fear this would be a quite a huge job though, and might prove to be
too rigid to adapt to otherwise valid code changes. While I'd love to
explore automated testing, I certainly don't have time to do anything
in this area I'm afraid with all the other things I need to work on
for Entangle :-(

Regards,
Daniel
--
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/NeoTech
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Gphoto-devel Digest, Vol 95, Issue 12

Theodore Kilgore


On Tue, 15 Apr 2014, Daniel P. Berrange wrote:

> On Tue, Apr 15, 2014 at 05:12:06PM -0500, Theodore Kilgore wrote:
> > I am very glad that someone has taken it upon himself to do this. It will
> > be nice to compile without generating all those warnings. My previous
> > impression, though, was that cruft had accumulated over time and that
> > nobody dared to touch this stuff, for fear that it might break something,
> > and somebody a long time ago wrote the code and is no longer here, etc.
>
> There's certainly scope to break stuff in a cleanup, but I think there's
> greater risk in not spotting real compile problems due to the large number
> of warnings hiding actual problems. Thus far the cleanups have been pretty
> straightforward though, so the risk is low.

Agreed, but I do hope that nothing gets broken.

>
> > I would like to make sure myself that everything is OK, but I won't have
> > time to concentrate on this until a couple of weeks from now, when the
> > academic term ends. In order to do the testing then, though, I would like
> > to ask that to what code base they are to be applied. In particular, I am
> > pretty sure I remember that there was to be a move away from SVN (and
> > possibly a move away from Sourceforge?) and the setting up of a git tree.
> > What is the status of this?
>
> Currently gphoto is still using SVN. That said I do my work using a hacked
> GIT conversion of the SVN repo since GIT is easier to work with for me.

OK, so I guess the short story is just go to the SVN tree.

>
> > 1. Often, inside of a camera driver it seems more stable to use unsigned
> > char than to use char in the various USB commands. I do not exactly know
> > why this happened, but I did notice. However, all the basic USB functions
> > are using char, not unsigned char. So in all or almost all of my camera
> > drivers the unsigned char variables have received casts.
>
> I think it would make sense that 'unsigned char *' be used everywhere,
> along with 'size_t' for any lengths. Internally most of the camera drivers
> use 'unsigned char*' for their own APIs. The problems, primarily arise
> when then interacting with the gp_port and the gp_file APIs which are
> using 'char *' when they really ought to have been 'unsigned char *'.

Exactly.

> The problem is the latter are in public header files, so changing
> them to 'nusigned char *' would be an API change that could well cause
> pain for applications using libgphoto. So I think we're stuck using
> casts.

In fact, the problem is even worse, in that libusb uses char and that in
turn is because the kernel uses char arguments in its USB-related
functions, too. Thus, it strikes me that libgphoto has not much choice,
and never had much choice. Everything underneath it as far as the USB
stack is concerned is using char instead of unsigned char. Why? It
probably goes back to the USB spec documents from there. But the reason
for that choice in the USB spec is not clear, either. So, ...

An even more amazing piece of legacy is that the names of some of the
Bayer tilings are not the same in libgphoto as they are in other places,
such as libv4l. And it appears to me that it is probably Gphoto which
somehow got these tiling names backwards way back when, many years ago.
But it is not obvious at this point how *that* can be reconciled. Much
better just to leave it alone. After all, it does not cause compiler
errors or warnings, only confusion for people writing code.


>
> > 2. As a case in point about ancient stuff that nobody can do much to
> > handle these days, consider the iclick camlib. I have only known of one
> > camera which is supported. It was onwned by someone in Spokane,
> > Washington (I live on the other side of the US, in Alabama), and i never
> > met him. The camlib was written based upon USB logs which were sent to me.
> > The owner of the camera (listed as co-author) disappeared from contact
> > many years ago. I have no idea how to contact him. Thus, there is no
> > feasible way to test revisions. I suspect that this is an extreme case,
> > but there are obviously similar ones here and there through the rest of
> > the camera libraries. How to make sure that everything is OK when there is
> > not camera to test and no known way to reach someone who owns one?

I could have added to this that I believe I could by now also have
provided a decompression algorithm for this camera, which has an optional
compressed mode. I think so, anyway, but I have no output from compressed
photos on which to test and no way to get any. So it is not even possible
to support every feature of that camera :-{

> Yeah, testing of cameras is a real hard problem. Even if you physical
> cameras, testing cameras manually is a real time sink. I think it might
> be possible to build a mock camera framework for testing. You'd have to
> do something like create a test script, run it against a real camera and
> record the USB traffic in some manner. Then the test suite would fake
> the USB interaction replaying the recorded USB traffic during the test.
> I fear this would be a quite a huge job though, and might prove to be
> too rigid to adapt to otherwise valid code changes. While I'd love to
> explore automated testing, I certainly don't have time to do anything
> in this area I'm afraid with all the other things I need to work on
> for Entangle :-(

The only problem with something like that is, one would need somebody
somewhere to have the actual camera and thus be in the position to create
the USB input which could be used for that particular camera. Another
camera of a different kind would require an analogous but different
set of data, it seems to me.

> Regards,
> Daniel

And thanks again for taking on this job. It is big enough as it is.

Theodore Kilgore

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/NeoTech
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Gphoto-devel Digest, Vol 95, Issue 12

Rogier Wolff
In reply to this post by Theodore Kilgore
On Tue, Apr 15, 2014 at 05:12:06PM -0500, Theodore Kilgore wrote:
> I have no idea how to contact him. Thus, there is no
> feasible way to test revisions. I suspect that this is an extreme case,
> but there are obviously similar ones here and there through the rest of
> the camera libraries. How to make sure that everything is OK when there is
> not camera to test and no known way to reach someone who owns one?

The thing is... The compiler issues a warning about a possible
problem. as an example: v = a + b * c; . We all know the precedence of
the operators so we all know that is v = a + (b*c). (with + and * the
compiler stays quiet, but with || and && it warns,
IIRC). Anyway,. ading the braces will silence the warning but not
change how the code is compiled. On the other hand, not testing means
that you might accidentally change something that does cause a bug....

        Roger.

--
** [hidden email] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
**    Delftechpark 26 2628 XH  Delft, The Netherlands. KVK: 27239233    **
*-- BitWizard writes Linux device drivers for any device you may have! --*
The plan was simple, like my brother-in-law Phil. But unlike
Phil, this plan just might work.

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/NeoTech
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel
Reply | Threaded
Open this post in threaded view
|

Re: Gphoto-devel Digest, Vol 95, Issue 12

Marcus Meissner-4
In reply to this post by Theodore Kilgore
Hi,

On Tue, Apr 15, 2014 at 05:12:06PM -0500, Theodore Kilgore wrote:
...

> Dan, Marcus,
>
> I am very glad that someone has taken it upon himself to do this. It will
> be nice to compile without generating all those warnings. My previous
> impression, though, was that cruft had accumulated over time and that
> nobody dared to touch this stuff, for fear that it might break something,
> and somebody a long time ago wrote the code and is no longer here, etc.
>
> I would like to make sure myself that everything is OK, but I won't have
> time to concentrate on this until a couple of weeks from now, when the
> academic term ends. In order to do the testing then, though, I would like
> to ask that to what code base they are to be applied. In particular, I am
> pretty sure I remember that there was to be a move away from SVN (and
> possibly a move away from Sourceforge?) and the setting up of a git tree.
> What is the status of this?

The state is still the old one... Unclear why Hans stopped working on this.

I would also still see a move to GIT at some point.

CIao, Marcus

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/NeoTech
_______________________________________________
Gphoto-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gphoto-devel