Re: [ovs-dev] [PATCH 03/10] windows: add includes to daemon-windows

[Alin Serdean]

> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Wednesday, April 19, 2017 7:46 AM
> To: Alin Serdean <aserd...@cloudbasesolutions.com>
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 03/10] windows: add includes to daemon-
> windows
> 
> On Wed, Apr 19, 2017 at 03:07:03AM +, Alin Serdean wrote:
> > > > -fprintf(filep_pidfile, "%d\n", _getpid());
> > > > +fprintf(filep_pidfile, "%d\n", getpid());
> > >
> > > This seems reasonable to me, except that usual practice would be
> > > more like
> > > this:
> > > fprintf(filep_pidfile, "%ld\n", (long int) getpid()); because
> > > pid_t might be short or int or long.
> > [Alin Serdean] Thanks for the review Ben, I totally missed that.
> > Although being MSFT specific if we replace getpid with
> GetCurrentProcessId (https://msdn.microsoft.com/en-
> us/library/windows/desktop/ms683180(v=vs.85).aspx) and from includes:
> > typedef unsigned long   DWORD;
> > I'm wondering if we should switch it to `%lu` and `(unsigned long)`.
> > What do you think?
> 
> Hmm.
> 
> POSIX says that pid_t must be a signed integer type.  An unsigned integer
> type for pid_t breaks things; for example, the POSIX wait() function has a
> pid_t return type and returns -1 to indicate an error.
> So it's somewhat distressing to have getpid() return an unsigned long, since
> that breaks real programs' fairly justified assumptions that
> getpid() returns a signed integer type.
> 
> What if, instead of "#define getpid() _getpid()", we used an inline function,
> e.g.
> 
> static inline pid_t getpid(void)
> {
> return GetCurrentProcessId();
> }
> 
> That would be cleaner from a POSIX point of view.
> 
> (On the other hand, if GetCurrentProcessId() might return a DWORD so big
> that it becomes a negative pid_t, we need to have a bigger discussion.)
[Alin Serdean] I agree it would look cleaner from a POSIX point of view, but I 
need to expand both functions to see if they call the same thing behind the 
scenes, so we can be sure it won't rollover. 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 03/10] windows: add includes to daemon-windows

[Ben Pfaff]

On Wed, Apr 19, 2017 at 03:07:03AM +, Alin Serdean wrote:
> > > -fprintf(filep_pidfile, "%d\n", _getpid());
> > > +fprintf(filep_pidfile, "%d\n", getpid());
> > 
> > This seems reasonable to me, except that usual practice would be more like
> > this:
> > fprintf(filep_pidfile, "%ld\n", (long int) getpid()); because pid_t 
> > might be
> > short or int or long.
> [Alin Serdean] Thanks for the review Ben, I totally missed that.
> Although being MSFT specific if we replace getpid with GetCurrentProcessId 
> (https://msdn.microsoft.com/en-us/library/windows/desktop/ms683180(v=vs.85).aspx)
>  and from includes:
> typedef unsigned long   DWORD;
> I'm wondering if we should switch it to `%lu` and `(unsigned long)`. 
> What do you think?

Hmm.

POSIX says that pid_t must be a signed integer type.  An unsigned
integer type for pid_t breaks things; for example, the POSIX wait()
function has a pid_t return type and returns -1 to indicate an error.
So it's somewhat distressing to have getpid() return an unsigned long,
since that breaks real programs' fairly justified assumptions that
getpid() returns a signed integer type.

What if, instead of "#define getpid() _getpid()", we used an inline
function, e.g.

static inline pid_t getpid(void)
{
return GetCurrentProcessId();
}

That would be cleaner from a POSIX point of view.

(On the other hand, if GetCurrentProcessId() might return a DWORD so big
that it becomes a negative pid_t, we need to have a bigger discussion.)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 03/10] windows: add includes to daemon-windows

[Alin Serdean]

> > -fprintf(filep_pidfile, "%d\n", _getpid());
> > +fprintf(filep_pidfile, "%d\n", getpid());
> 
> This seems reasonable to me, except that usual practice would be more like
> this:
> fprintf(filep_pidfile, "%ld\n", (long int) getpid()); because pid_t might 
> be
> short or int or long.
[Alin Serdean] Thanks for the review Ben, I totally missed that.
Although being MSFT specific if we replace getpid with GetCurrentProcessId 
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms683180(v=vs.85).aspx)
 and from includes:
typedef unsigned long   DWORD;
I'm wondering if we should switch it to `%lu` and `(unsigned long)`. 
What do you think?

Thanks,
Alin.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 03/10] windows: add includes to daemon-windows

[Ben Pfaff]

On Mon, Feb 06, 2017 at 04:41:40AM +, Alin Serdean wrote:
> Add fatal-signal.h include since it uses: fatal_signal_atexit_handler
> and fatal_signal_add_hook
> 
> Use the defined getpid() function and also include  since
> it is defined in include/windows/unistd.h .
> 
> Signed-off-by: Alin Gabriel Serdean 
> ---
>  lib/daemon-windows.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/daemon-windows.c b/lib/daemon-windows.c
> index 7e2e9da..ccf4297 100644
> --- a/lib/daemon-windows.c
> +++ b/lib/daemon-windows.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2014 Nicira, Inc.
> + * Copyright (c) 2014, 2017 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -20,7 +20,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "dirs.h"
> +#include "fatal-signal.h"
>  #include "ovs-thread.h"
>  #include "poll-loop.h"
>  #include "openvswitch/vlog.h"
> @@ -476,7 +478,7 @@ make_pidfile(void)
>  
>  fatal_signal_add_hook(unlink_pidfile, NULL, NULL, true);
>  
> -fprintf(filep_pidfile, "%d\n", _getpid());
> +fprintf(filep_pidfile, "%d\n", getpid());

This seems reasonable to me, except that usual practice would be more
like this:
fprintf(filep_pidfile, "%ld\n", (long int) getpid());
because pid_t might be short or int or long.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 03/10] windows: add includes to daemon-windows

[Sairam Venugopal]

Why should _getpid() be replaced with the other function? 

Thanks,
Sairam



On 2/5/17, 8:41 PM, "ovs-dev-boun...@openvswitch.org on behalf of Alin Serdean" 
 
wrote:

>Add fatal-signal.h include since it uses: fatal_signal_atexit_handler
>and fatal_signal_add_hook
>
>Use the defined getpid() function and also include  since
>it is defined in include/windows/unistd.h .
>
>Signed-off-by: Alin Gabriel Serdean 
>---
> lib/daemon-windows.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/lib/daemon-windows.c b/lib/daemon-windows.c
>index 7e2e9da..ccf4297 100644
>--- a/lib/daemon-windows.c
>+++ b/lib/daemon-windows.c
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (c) 2014 Nicira, Inc.
>+ * Copyright (c) 2014, 2017 Nicira, Inc.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
>@@ -20,7 +20,9 @@
> #include 
> #include 
> #include 
>+#include 
> #include "dirs.h"
>+#include "fatal-signal.h"
> #include "ovs-thread.h"
> #include "poll-loop.h"
> #include "openvswitch/vlog.h"
>@@ -476,7 +478,7 @@ make_pidfile(void)
> 
> fatal_signal_add_hook(unlink_pidfile, NULL, NULL, true);
> 
>-fprintf(filep_pidfile, "%d\n", _getpid());
>+fprintf(filep_pidfile, "%d\n", getpid());
> if (fflush(filep_pidfile) == EOF) {
> VLOG_FATAL("Failed to write into the pidfile %s", pidfile);
> }
>-- 
>2.10.2.windows.1
>___
>dev mailing list
>d...@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=Z6vowHUOjP5ysP_g372c49Nqc1vEKqHKNBkR5Q5Z7uo=4bES-cJ6lfbtKBVTx_l5djCuYUzfaAv1eo2-AJJVP4I=g49Hk4r9F29861GqEbXLFLjyYxmJENyhOos6Tr-BEdQ=
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev