Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Maintenance: Polish fqdncache.h #1848

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+3 −5
Open

Maintenance: Polish fqdncache.h #1848

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4952486
Maintenance: Polish fqdncache.h
yadij Jun 25, 2024
533a920
Apply suggestions from code review
yadij Jun 26, 2024
6da6c48
Merge branch 'master' into arc-dnsrr-2
yadij Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/fqdncache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,17 @@
#ifndef SQUID_SRC_FQDNCACHE_H
#define SQUID_SRC_FQDNCACHE_H

#include "ip/Address.h"
#include "dns/forward.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explicitly include ip/forward.h instead of starting to rely on current side effects of dns/forward.h inclusion (to get a forward declaration of Ip::Address).

Suggested change
#include "dns/forward.h"
#include "dns/forward.h"
#include "ip/forward.h"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dns/forward.h includes ip/forward.h , no need to include it twice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dns/forward.h includes ip/forward.h

Yes, my change request is based on this fact.

no need to include it twice.

To get the current code compiled, we do not need to include it explicitly (as already acknowledged in my change request).

To avoid relying on side effects, we do need to include it explicitly (as already stated in my change request). This reason is enough to include ip/forward.h explicitly despite the current code compiling without that explicit inclusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yet in #1848 (comment) you requested we rely on a side-effect of including the namespace Store forward declarations to also provide a global type.

Copy link
Contributor

@rousskov rousskov Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yet in #1848 (comment) you requested we rely on a side-effect of including the namespace Store forward declarations

I did not:

  • That change request is about using a reliable/direct/intended effect of store/forward.h header -- import of Store forward declarations. A foo/forward.h header exists exactly for that purpose -- it provides Foo forward declarations.
  • This change request is about avoiding current side effects of dns/forward.h header. A foo/forward.h header is not meant to be used for Bar forward declarations (and may not provide them or stop providing them).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your definition of "side-effects" in this case is wrongly applied. The dns/forward.h provides the declarations for libdns API. The storage type(s) used for DNS responses (eg. Ip::Address) are part of that and always will be - the definition(s) will be provided via dns/forward.h.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dns/forward.h provides the declarations for libdns API.

dns/forward.h should provide forward declarations for libdns API. Today, it also provides three function declarations, but that (hopefully temporary) violation is not to be relied on when making design decisions (not to mention relying on side effects of that violation!).

The storage type(s) used for DNS responses (eg. Ip::Address) are part of that and always will be

Ip::Address is not a part of libdns forward declarations today, and even if it becomes a part of them tomorrow, it is not something libdns users should rely on.

The relevant rule of thumb is quite simple: If header B.h uses a name declared in header A.h, then B.h should include either A/forward.h (if sufficient to get B.h compiled) or header A.h itself (otherwise).

#include "sbuf/forward.h"
#include "store/forward.h"

class StoreEntry;
namespace Dns
{
class LookupDetails;

/// whether to do reverse DNS lookups for source IPs of accepted connections
extern bool ResolveClientAddressesAsap;
}

typedef void FQDNH(const char *, const Dns::LookupDetails &details, void *);
typedef void FQDNH(const char *, const Dns::LookupDetails &, void *);

void fqdncache_init(void);
void fqdnStats(StoreEntry *);
Expand Down
Loading